Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Style ledger notifications like update notifications #3856

Merged
merged 4 commits into from
Sep 11, 2016
Merged

Style ledger notifications like update notifications #3856

merged 4 commits into from
Sep 11, 2016

Conversation

ayumi
Copy link
Contributor

@ayumi ayumi commented Sep 10, 2016 •
edited
Loading

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Partially implements #3257

  • Extract/refactor updateBar HTML and CSS for use in notificationBar
  • Add custom styling of notification bar and buttons

Auditors: @bradleyrichter @diracdeltas

Test Plan:

  1. Change conditional in https://github.com/brave/browser-laptop/blob/feature/ledger-notifications-update-style/app/ledger.js#L1220 to true
  2. Observe the funding notification is styled like the update bar

Preview:
screen shot 2016-09-09 at 17 26 40

@bradleyrichter
Copy link
Contributor

sweet! thanks!

@diracdeltas
Copy link
Member

i haven't looked into why, but this appears to be applying the style of the first notification bar to all the notification bars below it, regardless of whether they are ledger notifications.

to repro:

  1. go to https://www.browserleaks.com/geo, see the geolocation notification bar appear
  2. close browser, edit https://github.com/brave/browser-laptop/blob/feature/ledger-notifications-update-style/app/ledger.js#L1220 to true
  3. re-open browser
  4. observe that geolocation notification is styled like ledger notifications now
    screen shot 2016-09-09 at 6 14 22 pm

if you close the first notification so that the geolocation notification is on top, then the ledger notification loses its style
screen shot 2016-09-09 at 6 15 05 pm

otherwise i feel it

@ayumi
Copy link
Contributor Author

ayumi commented Sep 10, 2016

I initially tried to style each notification separately, but that looked really ugly due to variations in margin/padding.
So I decided it'd be an adequate fallback to style them all like the first notification.

Alternatively we could sort and group notifications by style, and separate them with borders on top/bottom.

@ayumi
Copy link
Contributor Author

ayumi commented Sep 10, 2016

I removed the style override based on the 0-th notification, and added borders. What do you think?

screen shot 2016-09-09 at 19 39 28

@diracdeltas
Copy link
Member

i feel it even more now

@bradleyrichter
Copy link
Contributor

We need to sync up the style on the yellow alert. Can we use 3 buttons instead?

No / yes / always

Or similar....?

On Sep 9, 2016, at 8:16 PM, yan zhu (@bcrypt) [email protected] wrote:

i feel it even more now

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@diracdeltas
Copy link
Member

We need to sync up the style on the yellow alert. Can we use 3 buttons instead?

It would have to be four buttons (No, Yes, Always No, Always Yes)

@ayumi ayumi force-pushed the feature/ledger-notifications-update-style branch from b8714f1 to a0e50c5 Compare September 10, 2016 06:18
@ayumi
Copy link
Contributor Author

ayumi commented Sep 10, 2016

@diracdeltas Notifications are now sorted by style and prioritize styled notifications over the defaults:
screen shot 2016-09-09 at 23 18 18

@bradleyrichter Besides restyling the 2 ledger notifications, this doesn't affect other notifications so I think it's out of scope for this PR.

@ayumi ayumi force-pushed the feature/ledger-notifications-update-style branch from a0e50c5 to d00fe90 Compare September 10, 2016 21:55
bsclifton
bsclifton reviewed Sep 10, 2016
View reviewed changes
js/stores/appStore.js

// Insert notification next to those with the same style, or at the end
let insertIndex = notifications.size
const style = action.detail.get
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for checking against immutable, you might check for the presence of toJS instead; I know a lot of other code does that (just a consistency nitpick)

@bsclifton
Copy link
Member

Comments left on the last commit 👍

diracdeltas
diracdeltas reviewed Sep 11, 2016
View reviewed changes
js/stores/appStore.js
// Insert notification next to those with the same style, or at the end
let insertIndex = notifications.size
const style = action.detail.get
? action.detail.get('options').get('style')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for future reference, you can just do thing.getIn(['options', 'style']) to avoid it throwing an error if thing.get('options') is falsy

@diracdeltas
Copy link
Member

this seems to cause some of the notification bar tests to fail. https://travis-ci.org/brave/browser-laptop/builds/159031606

otherwise lgtm, feel free to merge!

@ayumi ayumi force-pushed the feature/ledger-notifications-update-style branch from d00fe90 to 636c466 Compare September 11, 2016 22:07
@ayumi ayumi force-pushed the feature/ledger-notifications-update-style branch from 636c466 to b239577 Compare September 11, 2016 22:11
@ayumi
Copy link
Contributor Author

ayumi commented Sep 11, 2016

@diracdeltas thx, i've updated navigationBar test to use the new css selector

@ayumi ayumi merged commit 5e04180 into master Sep 11, 2016
@ayumi ayumi deleted the feature/ledger-notifications-update-style branch September 11, 2016 22:46
@ayumi ayumi mentioned this pull request Sep 11, 2016
Closed
@luixxiul luixxiul added this to the 0.12.1dev milestone Sep 12, 2016
This was referenced Sep 14, 2016
Closed
Closed
Closed
Closed
@alexwykoff alexwykoff mentioned this pull request Sep 15, 2016
Closed
@luixxiul luixxiul added design A design change, especially one which needs input from the design team. feature/rewards labels Sep 28, 2016
@luixxiul luixxiul mentioned this pull request Mar 23, 2017
Merged
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
design A design change, especially one which needs input from the design team. feature/rewards
Projects
None yet
Milestone
0.12.1dev
Development

Successfully merging this pull request may close these issues.
5 participants
@ayumi @bradleyrichter @diracdeltas @bsclifton @luixxiul